Various bug fixes. #163

michaelcfanning · 2025-03-03T22:20:08Z

BRK: Eliminate SEC000/101.Unclassified32CharacterString as noisy and not useful.
BRK: Rename SEC101/102.AdoPat friendly name to AdoLegacyPat.
BUG: Correct SEC000/002.Unclassified16ByteHexadecimalString id and rule name on calling GetMatchIdAndName (where SEC000/001.Unclassified64ByteBase64String was returned incorrectly before).
BUG: Resolve System.FormatException: The input is not a valid Base-46 string errors calling SEC101/102.AdoPat.GetMatchIdAndName by swallowing correct exception kind ArgumentException in IsChecksumValid helper.

I wish we could all be in a room together where I would invite everyone to hurl coffee cups, peanut shells, or anything else handy at my head. I completely failed to test all the code paths I strongly asserted I was covering in the last PR. Furthermore, Ross' confusion at a very strange set of conditions in a test I authored proved to be entirely warranted: these conditions indicate other bugs I simply wasn't seeing.

I am only human. Festina lente. I should have it tattooed in reverse on my forehead so that it's the first thing I see in the mirror every morning.

michaelcfanning · 2025-03-03T22:24:17Z

...ft.Security.Utilities.Core/PotentialSecurityKeys/SEC000_003_Unclassified32CharacterString.cs

-        /// The generated key is a 32-character string that contains alphanumeric characters
-        /// as well as symbols from the set: .=\-:[_@\*]+?
-        /// </summary>
-        public Unclassified32CharacterString()


Unclassified32CharacterString

Problem #1 in the strange test code paths Ross found suspicious: the test data generated by this rule was not in strict conformance across all its logic.

Basically, we have no general testing of the UnclassifiedPotentialSecurityKeys patterns list, which I could go slam into some other bucketing test. I am deferring that work for now.

For now, I'm ripping this rule out because it's noisy and not useful. The intent here was to capture an ancient variation of an generated AAD SPN secret, then the rule was renamed as a generic Unclassified32CharacterString because it fired too frequently and really it should have been deleted entirely. If there's an analysis here, it will argue its way back in.

michaelcfanning · 2025-03-03T22:25:43Z

...urity.Utilities.Core/PotentialSecurityKeys/SEC000_002_Unclassified16ByteHexadecimalString.cs

@@ -22,7 +22,7 @@ public Unclassified16ByteHexadecimalString()
        DetectionMetadata = DetectionMetadata.HighEntropy | DetectionMetadata.Unclassified | DetectionMetadata.LowConfidence;
    }

-    public override Tuple<string, string>? GetMatchIdAndName(string match) => new Tuple<string, string>("SEC000/001", "Unclassified64ByteBase64String");
+    public override Tuple<string, string>? GetMatchIdAndName(string match) => new Tuple<string, string>("SEC000/002", nameof(Unclassified16ByteHexadecimalString));


002

boo. cut and paste error. I added invariant tests that ensure no rules collide with each other in core metadata expression and also that GetMatchIdAndName results always match the declared rule id and name.

This is what we get for being 'cute' and allowing a single rule to be flexible in what ids it covers. That's useful but it tortures the design more than a little and leaves us vulnerable to bugs of this kind.

michaelcfanning · 2025-03-03T22:26:03Z

src/Microsoft.Security.Utilities.Core/PreciselyClassifiedSecurityKeys/SEC101_102_AdoPat.cs

@@ -5,14 +5,14 @@

 namespace Microsoft.Security.Utilities
 {
-    public class AdoPat : RegexPattern
+    public class AdoLegacyPat : RegexPattern


AdoLegacyPat

This rule no longer applies to the latest ADO PAT format and thefore I'm renaming it.

michaelcfanning · 2025-03-03T22:28:10Z

src/Tests.Microsoft.Security.Utilities.Core/AdoLegacyPatTests.cs

+    public class AdoLegacyPatTests
+    {
+        [TestMethod]
+        public void AdoLegacyPat_InvalidBase32Input()


AdoLegacyPat_InvalidBase32Input

Boo. For all my talk of test-driven development in the last PR, I ended up not shipping a correct fix! The reason? Late-breaking changes to a test I authored altered the code coverage and then at the very end a conversion from generated exception type (from FormatException to ArgumentException) left the code in a crashing condition,

Sigh. Even while waiting on a good unit test pattern, I should have started with simple, atomic tests to reveal bugs and prove they are fixed. Duly chastened, I return here with this review. #Resolved

Sigh. Even while waiting on a good unit test pattern, I should have started with simple, atomic tests to reveal bugs and prove they are fixed. Duly chastened, I return here with this review.

❤️

src/Tests.Microsoft.Security.Utilities.Core/WellKnownRegexPatternsTests.cs

src/Microsoft.Security.Utilities.Core/WellKnownRegexPatterns.cs

src/Tests.Microsoft.Security.Utilities.Core/WellKnownRegexPatternsTests.cs

rwoll

The invariant test for no duplicate ids looks like it's missing a key piece. See comments.

Otherwise, just some nits.

GeneratedRegexPatterns/LowConfidenceSecurityModels.json

rwoll · 2025-03-04T01:23:02Z

src/Tests.Microsoft.Security.Utilities.Core/AdoLegacyPatTests.cs

+    public class AdoLegacyPatTests
+    {
+        [TestMethod]
+        public void AdoLegacyPat_InvalidBase32Input()


Sigh. Even while waiting on a good unit test pattern, I should have started with simple, atomic tests to reveal bugs and prove they are fixed. Duly chastened, I return here with this review.

❤️

rwoll · 2025-03-04T01:25:17Z

src/Tests.Microsoft.Security.Utilities.Core/AdoLegacyPatTests.cs

+            var classifier = new AdoLegacyPat();
+            string invalidInput = "=22222222222222222222222222";
+            var result = classifier.GetMatchIdAndName(invalidInput);
+            Assert.IsNull(result);


Yay! This is a very easy to understand test <3.

For due diligence, if you have not already, run this test attached with a debugger and ensure the null is resulting from the expected error line getting hit. i.e. this is null for a particular reason, and not a general indeterminate reason.

Depending on perf constraints, there's more formal methods to handle this kinda testing — e.g. internal logging and asserting on specific logs are hit even if everything publicly is turned into a null. Out of scope for this PR, of course, but food for thought. #Resolved

src/Tests.Microsoft.Security.Utilities.Core/WellKnownRegexPatternsTests.cs

rwoll · 2025-03-04T01:35:38Z

src/Tests.Microsoft.Security.Utilities.Core/WellKnownRegexPatternsTests.cs

+            HashSet<string> ruleIds = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
+            HashSet<string> ruleNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase);


Suggested change

HashSet<string> ruleIds = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

HashSet<string> ruleNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

HashSet<string> seenRuleIds = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

HashSet<string> seenRuleNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

nit: these renames will make the loop code easier to read #Resolved

src/Tests.Microsoft.Security.Utilities.Core/WellKnownRegexPatternsTests.cs

rwoll · 2025-03-04T01:38:14Z

src/Tests.Microsoft.Security.Utilities.Core/WellKnownRegexPatternsTests.cs

+                result = ruleIds.Contains(pattern.Id);
+                result.Should().BeFalse(because: $"Pattern '{pattern.GetType().Name}' should not share its Id with another rule: '{pattern.Id}'");
+
+                result = ruleNames.Contains(pattern.Name);


Suggested change

result = ruleNames.Contains(pattern.Name);

bool duplicatePatternName = ruleNames.Contains(pattern.Name);

nit: instead of using generic result everywhere some more specific names will increase readability. #WontFix

I don't do this. I set result to my test condition so that it's on one line. Then I follow it with an assertion with a strong readable message.

result isn't intended to encode something useful, the detailed, highly authored message it.

i could allocate new variables with names but this tends to consume more space and i worry about readability.

let's punt for now i will think about your suggestion.

I read the code top to bottom. A well-named variable means I know how to read/check the line without the out-of-order flow.

Better yet if you want the because to be the sole communicator, skip using the variable altogether:

alreadySeeanRullName.Should().Not().Contains(pattern.Name, because: "…");

These are nits, so keep thinking on it.

It is a good suggestion to encode more information about the test expectation in the variable name, I will keep this in mind moving forward.

Dismissing as not to block as bug is in unrelated test code.

src/Microsoft.Security.Utilities.Core/CachedDotNetRegex.cs

src/Microsoft.Security.Utilities.Benchmarks/RE2RegexEngine.cs

src/Microsoft.Security.Utilities.Core/CachedDotNetRegex.cs

src/Microsoft.Security.Utilities.Benchmarks/RE2RegexEngine.cs

docs/ReleaseHistory.md

…curity-utilities into 1-16-0-back-to-main

src/Tests.Microsoft.Security.Utilities.Core/DetectionTests.cs

src/Tests.Microsoft.Security.Utilities.Core/RE2RegexEngine.cs

src/Tests.Microsoft.Security.Utilities.Core/WellKnownRegexPatternsTests.cs

Various bug fixes.

47072de

michaelcfanning requested review from cfaucon, suvamM, rwoll and nguerrera as code owners March 3, 2025 22:20

michaelcfanning commented Mar 3, 2025

View reviewed changes

nguerrera reviewed Mar 3, 2025

View reviewed changes

src/Tests.Microsoft.Security.Utilities.Core/WellKnownRegexPatternsTests.cs Show resolved Hide resolved

Update version.json to v1.16.0

b944601

nguerrera reviewed Mar 3, 2025

View reviewed changes

src/Microsoft.Security.Utilities.Core/WellKnownRegexPatterns.cs Outdated Show resolved Hide resolved

nguerrera reviewed Mar 3, 2025

View reviewed changes

src/Tests.Microsoft.Security.Utilities.Core/WellKnownRegexPatternsTests.cs Show resolved Hide resolved

nguerrera previously approved these changes Mar 3, 2025

View reviewed changes

Remove unnecessary double fire check

5cea309

michaelcfanning dismissed nguerrera’s stale review via 5cea309 March 3, 2025 22:36

PR feedback

33f8a72

nguerrera previously approved these changes Mar 3, 2025

View reviewed changes

rwoll previously requested changes Mar 4, 2025

View reviewed changes

rwoll self-requested a review March 4, 2025 01:40

Restore RE2 '?P<refine>' named group pattern.

4193f31

michaelcfanning dismissed nguerrera’s stale review via 4193f31 March 4, 2025 19:44

Update release notes

635fc0d

nguerrera reviewed Mar 4, 2025

View reviewed changes

src/Microsoft.Security.Utilities.Core/CachedDotNetRegex.cs Outdated Show resolved Hide resolved

nguerrera reviewed Mar 4, 2025

View reviewed changes

src/Microsoft.Security.Utilities.Benchmarks/RE2RegexEngine.cs Outdated Show resolved Hide resolved

nguerrera reviewed Mar 4, 2025

View reviewed changes

src/Microsoft.Security.Utilities.Benchmarks/RE2RegexEngine.cs Outdated Show resolved Hide resolved

nguerrera reviewed Mar 4, 2025

View reviewed changes

src/Microsoft.Security.Utilities.Core/CachedDotNetRegex.cs Outdated Show resolved Hide resolved

nguerrera reviewed Mar 4, 2025

View reviewed changes

src/Microsoft.Security.Utilities.Benchmarks/RE2RegexEngine.cs Outdated Show resolved Hide resolved

nguerrera reviewed Mar 4, 2025

View reviewed changes

docs/ReleaseHistory.md Outdated Show resolved Hide resolved

michaelcfanning added 3 commits March 4, 2025 12:15

Add test file.

5d2244e

Add named group test.

05cb6da

PR feedback

12eb88c

michaelcfanning closed this Mar 4, 2025

michaelcfanning added 8 commits March 4, 2025 14:06

Merge branch 'main' into 1-16-0-back-to-main

1be2e59

Remove duplicated class source

135f2f0

Merge branch '1-16-0-back-to-main' of https://github.com/microsoft/se…

434f5ff

…curity-utilities into 1-16-0-back-to-main

Restore RE2 compatible named group for CASK rule

32ac22e

Fix release notes

1ce7970

Restore commented test

7b1e144

Eliminate nullable warning

3501929

Fix prefix to be RE2 compatible in high performance code path

176b6bd

rwoll mentioned this pull request Mar 5, 2025

Fixes for upcoming 1.16.0 release #165

Closed

Test resilience

bcd3be7

michaelcfanning reopened this Mar 5, 2025

michaelcfanning added 2 commits March 5, 2025 08:29

Release history clean-up

c326d78

Add comment for memory cache value.

3812c56

nguerrera reviewed Mar 5, 2025

View reviewed changes

src/Tests.Microsoft.Security.Utilities.Core/DetectionTests.cs Show resolved Hide resolved

nguerrera reviewed Mar 5, 2025

View reviewed changes

src/Tests.Microsoft.Security.Utilities.Core/RE2RegexEngine.cs Outdated Show resolved Hide resolved

nguerrera reviewed Mar 5, 2025

View reviewed changes

src/Tests.Microsoft.Security.Utilities.Core/WellKnownRegexPatternsTests.cs Outdated Show resolved Hide resolved

nguerrera previously approved these changes Mar 5, 2025

View reviewed changes

PR feedback on test patterns.

50b5f57

michaelcfanning dismissed nguerrera’s stale review via 50b5f57 March 5, 2025 17:32

Test syntax error

1525550

nguerrera approved these changes Mar 5, 2025

View reviewed changes

michaelcfanning merged commit fee7a62 into main Mar 5, 2025
12 checks passed

michaelcfanning deleted the ado-pat-exception branch March 5, 2025 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various bug fixes. #163

Various bug fixes. #163

michaelcfanning commented Mar 3, 2025

michaelcfanning Mar 3, 2025

michaelcfanning Mar 3, 2025

michaelcfanning Mar 3, 2025

michaelcfanning Mar 3, 2025 •

edited

Loading

rwoll Mar 4, 2025

rwoll left a comment

rwoll Mar 4, 2025

rwoll Mar 4, 2025 •

edited by michaelcfanning

Loading

rwoll Mar 4, 2025 •

edited by michaelcfanning

Loading

rwoll Mar 4, 2025 •

edited by michaelcfanning

Loading

michaelcfanning Mar 4, 2025

rwoll Mar 4, 2025

michaelcfanning Mar 5, 2025

		HashSet<string> ruleIds = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
		HashSet<string> ruleNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

	result = ruleNames.Contains(pattern.Name);
	bool duplicatePatternName = ruleNames.Contains(pattern.Name);

Various bug fixes. #163

Various bug fixes. #163

Conversation

michaelcfanning commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwoll Mar 4, 2025 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

rwoll Mar 4, 2025 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

rwoll Mar 4, 2025 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Mar 3, 2025 •

edited

Loading

rwoll Mar 4, 2025 •

edited by michaelcfanning

Loading

rwoll Mar 4, 2025 •

edited by michaelcfanning

Loading

rwoll Mar 4, 2025 •

edited by michaelcfanning

Loading